Move proxy values balancing to the beginning of parsing #8133

javorszky · 2025-08-13T21:32:54Z

Proposed changes

This PR moves the proxy values balancing from the template helpers to the time where the annotations and config maps are being parsed. There are a few notable changes:

proxy value types (like 4 8k and 16k) are now custom structs with iota constants for the unit (k, m), and uint64 for the numbers. This guarantees that once the values are parsed and corrected, they won't become invalid later through any changes, and we can offload a lot of the error checking to the compiler. One such is that we know none of the values will be negative, because a uint can't be negative
some tests had to be changed. For example previously an empty proxy_buffers value passed in via a configmap would generate a bad config, now it gets auto-corrected to the default 2 4k value. This is the cost of moving the proxy balancing to be right after parsing the values
similarly the e2e pytest also had to change because if the passed in values are actually invalid, then the result will have those configuration items missing, rather than present, but with invalid values
I brought on and reused most of the tests that were in the PR originally, though some of them had to be changed. The logic of what value stays, and what values move around it was fuzzy. Right now if proxy-buffers is not defined, that gets set to the default value of 2 4k, and everything else gets brought down to fit into that one
rules of how the values relate to each other are based on the NGINX source code
- proxy_buffers: https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L3763-L3767
- proxy_busy_buffers_size: https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L3787-L3804
those rules also mean that the proxy_buffers_size is at most all of proxy_buffers minus one buffer size

This PR targets @AlexFenlon 's PR, as it's a modification of that one. Once merged, then we can bring the branch up to date with main.

Questions

Currently I put the custom data type and its tests into the internal/validation package, however there's also the pkg/apis/configuration/v1/types.go file which seems to do something similar. I have lukewarm feelings about where this should live, but the ambiguity around what v1 and v2 mean means that I'm unsure whether I want to put the custom data type in this PR into the pkg/apis/configuration pkg.

Namewise I should probably have named the SizeWithUnit and NumberSizeConfig something more clever. Help there is appreciated.

Tests

Because PRs to non-main and non-release branches don't run CI, I manually ran the unit tests with make test, those passed.

I also manually ran the one failing e2e test with the following after building a fresh image and loading that into minikube:

make run-tests-in-minikube BUILD_IMAGE=nginx/nginx-ingress:5.2.0-SNAPSHOT NODE_IP=$(minikube ip) PYTEST_ARGS='-sv suite/test_virtual_server_configmap_keys.py::TestVirtualServerConfigMapNoTls'

I'm currently running ALL of the e2e tests on a debian image. That might take a while, so I'll return to it once it finished and update this part of the PR message.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

…nabled and vise versa

…e-buffer-config

…at/safe-buffer-config # Conflicts: # internal/configs/commonhelpers/common_template_helpers.go # internal/configs/commonhelpers/common_template_helpers_test.go

pdabelf5 · 2025-08-14T08:12:12Z

pkg/apis/configuration/v1/types.go

afaik should only be used for CRD generation

codecov · 2025-08-14T08:25:48Z

Codecov Report

❌ Patch coverage is 71.31783% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.33%. Comparing base (d406b39) to head (0186bb5).

Files with missing lines	Patch %	Lines
internal/configs/annotations.go	16.12%	23 Missing and 3 partials ⚠️
internal/validation/validation.go	60.37%	19 Missing and 2 partials ⚠️
internal/validation/data_types.go	85.60%	14 Missing and 5 partials ⚠️
internal/configs/configmaps.go	77.14%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8133      +/-   ##
==========================================
+ Coverage   53.11%   53.33%   +0.21%     
==========================================
  Files          90       91       +1     
  Lines       21778    22024     +246     
==========================================
+ Hits        11567    11746     +179     
- Misses       9736     9790      +54     
- Partials      475      488      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AlexFenlon · 2025-08-14T11:29:17Z

Docs PR: nginx/documentation#917

Copilot

Pull Request Overview

This PR moves proxy buffer value balancing from template helpers to the config/annotation parsing phase. It introduces custom data types for size and buffer configurations to ensure type safety and consistency, along with validation rules based on NGINX source code requirements.

Key changes:

Introduces SizeWithUnit and NumberSizeConfig structs with type-safe parsing and validation
Implements BalanceProxyValues function to normalize proxy buffer relationships according to NGINX constraints
Updates test expectations to handle auto-correction of invalid proxy buffer configurations

Reviewed Changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`internal/validation/data_types.go`	Defines new custom types for size and buffer configurations with validation logic
`internal/configs/configmaps.go`	Updates config parsing to use new types and balance proxy values
`internal/configs/annotations.go`	Updates annotation parsing to use new types and balance proxy values
`tests/suite/test_virtual_server_configmap_keys.py`	Updates test to handle corrected proxy buffer behavior
`internal/configs/virtualserver.go`	Updates template generation to use string representation of new types

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-14T13:01:00Z

internal/validation/validation.go

+	}
+
+	// Autocorrect invalid units to 'm'
+	if unit != 'k' && unit != 'm' {


The autocorrection logic that changes invalid units to 'm' is not documented in the function comment. This behavior should be clearly documented as it could be unexpected for users.

Copilot · 2025-08-14T13:01:01Z

internal/validation/data_types.go

+	}
+
+	if num > 1024 {
+		ret.Size = 1024


The magic number 1024 should be defined as a named constant to improve code readability and maintainability.

Suggested change

ret.Size = 1024

if num > MaxAllowedSize {

ret.Size = MaxAllowedSize

Copilot · 2025-08-14T13:01:01Z

internal/validation/data_types.go

+	}
+
+	if num < 2 {
+		num = 2


The magic number 2 (minimum number of buffers) should be defined as a named constant to improve code readability and document the NGINX requirement.

Suggested change

num = 2

if num < MinNginxBufferCount {

num = MinNginxBufferCount

Copilot · 2025-08-14T13:01:01Z

internal/validation/data_types.go

+	// 1.b there must be at most 1024 proxy buffers
+	if proxyBuffers.Number > 1024 {
+		modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to 1024", proxyBuffers.Number))
+		proxyBuffers.Number = 1024


The magic number 1024 appears again and should be defined as a named constant to avoid duplication and improve maintainability.

Suggested change

proxyBuffers.Number = 1024

// 1.b there must be at most MaxProxyBuffers proxy buffers

if proxyBuffers.Number > MaxProxyBuffers {

modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to %d", proxyBuffers.Number, MaxProxyBuffers))

proxyBuffers.Number = MaxProxyBuffers

Copilot · 2025-08-14T13:01:02Z

internal/configs/configmaps.go

+		} else {
+			cfgParams.ProxyBufferSize = proxyBufferSizeData
+		}
+		// normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize)


This commented out code should be removed as it's not being used and adds clutter to the codebase.

Suggested change

// normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize)

Copilot · 2025-08-14T13:01:02Z

internal/configs/configmaps.go

+		}
+	}
+
+	// Normalise the three proxy buffer values across each other.


This comment doesn't add value as it just restates what the code above does. It should be removed or made more descriptive.

Suggested change

// Normalise the three proxy buffer values across each other.

// Ensure proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size are consistent.

// The BalanceProxyValues function reconciles these values to prevent misconfiguration,

// applying necessary adjustments and logging any changes made.

AlexFenlon and others added 30 commits July 24, 2025 13:35

add helper function to enable proxy_buffers if proxy_buffer_size is e…

252ee58

…nabled and vise versa

add a safe way to enable proxy_buffers and proxy_buffer_size

4d5a6d3

add nginx.org/proxy-busy-buffers-size annotation to ingress

36ad41a

add proxy-busy-buffer-size to configmap, improve validation

0952f77

Merge remote-tracking branch 'refs/remotes/origin/main' into feat/saf…

5bea282

…e-buffer-config

rename ProxyBusyBufferSize to ProxyBusyBuffersSize to be more accurate

de8c064

Add ProxyBusyBuffersSize to VirtualServer as busy-buffers-size

ca2e16b

make validation better

4162d5d

Merge branch 'main' into feat/safe-buffer-config

7457a49

make validation better

1bf7b14

Merge remote-tracking branch 'origin/feat/safe-buffer-config' into fe…

9cffde2

…at/safe-buffer-config # Conflicts: # internal/configs/commonhelpers/common_template_helpers.go # internal/configs/commonhelpers/common_template_helpers_test.go

Update tests

10999ff

Update tests, improve validation

b9394c0

fix validation

58fe7cd

Put back the proxy max temp filesize data

942f17d

Create a new typed unit to store sizes and offsets

bd2cd8e

Move new units to internal configs

e2a9072

Move data types to their own file in internal/validation

22187df

Move proxy units into common package

eb0098d

Adjust templates to make them pass

4abb3cf

Normalise parsing of bad / unknown to mb

64a401c

Return emptystring if numbersize is zero

d8998cd

Create BalanceProxy function and tests

cd5587b

Reconcile the proxy values after parsing them

af0aba1

Fix logic in clamping behaviour

aee9418

Fix tests, make another test parallel

ed52d20

Account for proxies not being present if invalid

2a8287f

Simplify tests for data types

254c261

Fix tests

ba838f0

Fix more tests

128e97a

javorszky requested a review from a team as a code owner August 13, 2025 21:32

github-actions bot added enhancement Pull requests for new features/feature enhancements go Pull requests that update Go code python Pull requests that update Python code labels Aug 13, 2025

javorszky changed the base branch from feat/safe-buffer-config to main August 14, 2025 08:21

Merge branch 'main' into feat/fixed-safe-buffer-config

0186bb5

AlexFenlon mentioned this pull request Aug 14, 2025

Add Safer Proxy_Buffer Config #8088

Closed

6 tasks

AlexFenlon linked an issue Aug 14, 2025 that may be closed by this pull request

Safe proxy buffer configuration #8004

Open

5 tasks

Merge branch 'main' into feat/fixed-safe-buffer-config

c55f83d

danielnginx requested a review from Copilot August 14, 2025 12:58

Copilot AI reviewed Aug 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move proxy values balancing to the beginning of parsing #8133

Move proxy values balancing to the beginning of parsing #8133

Uh oh!

javorszky commented Aug 13, 2025 •

edited

Loading

Uh oh!

pdabelf5 commented Aug 14, 2025

Uh oh!

codecov bot commented Aug 14, 2025

Uh oh!

AlexFenlon commented Aug 14, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Copilot AI Aug 14, 2025

Uh oh!

Uh oh!

	ret.Size = 1024
	if num > MaxAllowedSize {
	ret.Size = MaxAllowedSize

	num = 2
	if num < MinNginxBufferCount {
	num = MinNginxBufferCount

-		proxyBuffers.Number = 1024
+	// 1.b there must be at most MaxProxyBuffers proxy buffers
+	if proxyBuffers.Number > MaxProxyBuffers {
+		modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to %d", proxyBuffers.Number, MaxProxyBuffers))
+		proxyBuffers.Number = MaxProxyBuffers

-	// Normalise the three proxy buffer values across each other.
+	// Ensure proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size are consistent.
+	// The BalanceProxyValues function reconciles these values to prevent misconfiguration,
+	// applying necessary adjustments and logging any changes made.

Move proxy values balancing to the beginning of parsing #8133

Are you sure you want to change the base?

Move proxy values balancing to the beginning of parsing #8133

Uh oh!

Conversation

javorszky commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Questions

Tests

Checklist

Uh oh!

pdabelf5 commented Aug 14, 2025

Uh oh!

codecov bot commented Aug 14, 2025

Codecov Report

Uh oh!

AlexFenlon commented Aug 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javorszky commented Aug 13, 2025 •

edited

Loading